Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Streamline has_trait_flag behavior #2911

Merged
merged 32 commits into from
Sep 26, 2023

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented May 29, 2023

Summary

SUMMARY: Performance "Rework has_trait_flag to check mutation cache instead of all traits, overhaul with support for trait flag strings leading to IDs instead of using std::string"

Purpose of change

Back while I was working on #1486 it was brought up that has_trait_flag evidently taxes performance moreso than has_trait, with this being a blocker.

Since I like JSONizing hardcoded trait effects so that modders can do cool stuff with them, it would be a good idea to make has_trait_flag have comparable performance impact as has_trait. It turned out to be significantly more complicated due to how flags are (or really, aren't) handled under the hood, but the original idea is still buried in there alongside the bigger changes per Coolthulhu's suggestion. In short, mutation flags had to be defined as a whole separate thing that the code can look up in more efficiency ways than the usual method that strings are grabbed for all other flag checks.

Describe the solution

JSON Changes:

  • Created flags_mutation.json, being used to set aside a mutation_flag entry for every trait flag currently used by the code. Also went ahead and defined the mycus flag used for dialogue screening of Marloss mutations and the flags used only by cosmetic hair traits.
  • Removed NON_TRESH from the regular flags.json, being the only trait flag with an entry there.

C++ Changes:

  • In activity_handlers.cpp, set the trait flag definitions used elsewhere in the file to look for trait_flag_str_id instead of mere strings.
  • In character.cpp, updated Character::practice so that the checks for the pred trait flags are formatted to work with the new trait flags, doing the same with Character::apply_skill_boost now with removing the self-contained definitions of flags that probably should be moved to the top of the file if they're going to be kept (and might still be, as per alternatives section). Character::mod_thirst, Character::mod_rad, and Character::set_highest_cat_level were all also updated so that uses of has_trait_flag in them all asked for trait_flag_str_id instead of a raw string.
  • In character.h, updated its definition of has_trait_flag to ask for trait_flag_id instead of std::string.
  • In condition.cpp, set the set_has_trait_flag check so that it feeds trait_flag_to_check to has_trait_flag as a trait flag string instead of just a string.
  • In consumption.cpp, set Character::will_eat so that its use of a trait flag check uses a trait flag string instead of a string.
  • Added flag_trait.cpp, derived from the contents of flag.cpp to define trait flags as something more than just a simple std::string. Per Coolthulhu's advice in this PR and on the discord, this establishes the JSON to be checked and checking that the code is using trait flags that actually exist in the JSON. This is what defines trait_flag_id as IDs for trait flag checks in the code to use, and trait_flag_str_id as strings for trait flags that can be checked and handled in whatever ways it needs that just being fed std::string wholesale won't allow.
  • Added flag_trait.h, likewise based of flag.cpp.
  • In init.cpp, added include for flag_trait.h, updated DynamicDataLoader::initialize to tell it that mutation_flag is a type of JSON entry that exists, and added relevant checks for flag_trait stuff to DynamicDataLoader::unload_data, DynamicDataLoader::finalize_loaded_data, and DynamicDataLoader::check_consistency.
  • In item.cpp, set the definition of a trait flag used in the file to give it a trait_flag_str_id instead of std::string.
  • In iuse_actor.cpp, updated checks for trait flag strings in enzlave_actor::use.
  • In magic.cpp, updated checks for trait flag strings in spell::spell_fail.
  • In melee.cpp, updated checks for trait flag strings in Character::roll_bash_damage, Character::roll_cut_damage, and Character::roll_stab_damage.
  • In mondeath.cpp, updated checks for trait flag strings in mdeath::guilt.
  • In mutation.cpp, updated Character::has_trait_flag so it asks for a trait_flag_id instead of std::string. In addition, as per the original focus of this PR, it's set to check cached_mutations instead of calling get_mutations, with the expectation that this will have some improvement in performance should more uses of traitflag checks be added to the code.
  • In mutation.h, set it so that mutations look for trait_flag_str_id instead of std::string when they ask for flags.
  • In mutation_data.cpp, set mutation_branch::load so that loading mutation flags correctly tries to read trait flag strings instead of using string_reader. For now I've kept it so the JSON in mutations still says flags instead of changing it, to save some editing of mutation JSON and so modders don't have any mods that add mutations with flags instantly broken by this PR.
  • In type_id.h, added a class for json_trait_flag.

Documentation Changes:

  • Added all flags in use in the hardcode to JSON_FLAGS.md along with an explanation of them being a different sorta JSON.

Describe alternatives you've considered

  • Converting all uses of trait flag strings in the code to pre-defined examples at the top of the file, so they don't have to define the trait_flag_str_id multiple times per file. For now I've kept usage in the files consistent with original usage, this could be done in this PR or a followup, whatever would be preferred.
  • Setting it so that mutations call for trait flags using something other than flags. This would help reduce confusion about the fact that mutation flags are a whole different type of JSON thing from other flags, but as noted earlier it would increase the amount of minor JSON edits and modders would have to update any mutations they have that use flags. With this, all the work is already done on our end and any flags used by the code will still behave the same for those modders.
  • Giving Sapiovore the SAPIOVORE flag and changing whatever trait flag checks we need to to make it actually have an in-repo use in mutation JSON. No idea what we'd even what it to do separately from Cannibal though.
  • JSONize all the things!

Testing

  • Compiled and load-tested.
  • Tested that butchering a dead NPC warns about descrating the dead without relevant mutations, and that this goes away if you have Cannibal, Psychopath, or Sapiovore.
  • Tested whaling on a debug monster both with and without Apex Predator, confirmed that focus would tank without it but stay at normal levels with it.
  • Also confirmed that punching the debug monster gave 5% to melee skill and 9% to unarmed with the very first hit, while with Apex Predator the amounts for the first hit were 9% and 15%
  • Waited around a while with one of the relevant predator traits to confirm the "you recall your most recent hunt" message still shows up.
  • Tested temporarily adding NO_THIRST and NO_RADIATION flags to a trait, tested sitting around in a HWS for a while both with and without that trait, confirming that both work as intended.
  • Checked that adding Killer Drive incremented Lupine mutation category normally (showed as 8), tested adding the NON_THRESH flag to it and confirmed that it no longer counted as adding to mutation category strength.
  • Messed with forcing marloss cultists to spawn, added the Spiritual trait, confirmed that when you talk to them you can pick a fight with them by calling their wacky shenanigans heretical, and that this dialogue option disappears if you have Marloss Carrier, thus confirming that NPC dialogue use of has_trait flag still works.
  • Ate some delicious people meat, as intended if you lack the Cannibal trait the warning message notes that eating people is mean.
  • Confirmed that item screen for human flesh states the obvious, in red if you lack the relevant trait, in green if you have it.
  • Spawned in a zombie and killed it, tested the morale penalty needed to become uncomfortable with neutering it, confirmed that Predator and Apex Predator compensates. Likewise confirmed that having Culler takes a bite out of that 250 morale penalty you'd normally get, while doing this with Apex Predator means no morale hit at all.
  • Tested giving myself Rat Claws and wailing on a debug monster. Unarmed hits leaned towards the 10-12 range at zero unarmed skill, 20-22 at 10 unarmed skill. Tested that damage goes back down if the UNARMED_BONUS flag is removed from Rat Claws.
  • Tested turning a zombie child into a delicious red smear with mjolnir, PC gets the smol sad if no traits, gets the message about culling the weak with Culler trait (and morale hit is smaller), no message with Predator.
  • Recruited starter NPC, confirmed that I could ask them about their backstory without any issues.
  • Tested editing the evac merchant so you can't trade with him without a mutation threshold, confirmed that use of trait flag checking also works without issue, and that it's a special case where the trait flag doesn't have to be defined in JSON

Dumb thing: Culler and Hunter reduce the morale penalty from enzlaving a zombie, and Apex Predator eliminates it outright, but PRED3 is in a derpy weird spot where it increases your tolerance for being willing to start mutilating a zombie, but does nothing for how massive a penalty you take from it like its preceding traits do.

I don't actually know how to confirm how much better this performs than using get_mutation...

Additional context

@github-actions github-actions bot added the src changes related to source code. label May 29, 2023
scarf005
scarf005 previously approved these changes May 29, 2023
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we could use std::find_if.

@chaosvolt
Copy link
Member Author

Could also be handy, not sure if it's used in the code at present.

@scarf005
Copy link
Member

image
i think so.

@Coolthulhu
Copy link
Member

Coolthulhu commented May 30, 2023

It needs a conversion to proper id'd object, with generic factory and all. Having the flags be strings is not very maintainable.

EDIT: Maybe it's possible to reuse the flag class used for item flags

@chaosvolt
Copy link
Member Author

I'm not really sure how to do that is the problem. :/

@Coolthulhu
Copy link
Member

  • CTRL+C+V flag.h/.cpp
  • Name them something like "flag_trait"
  • Hook load and verify up in init.cpp (look at how the current item flags are used there and copy that)
  • Change the type of trait flags to string_id<flag_trait> (probably in a set, so like std::set<string_id<flag_trait>>
  • In trait verify (I think it's something like mutation_branch::verify) or finalize (if verify doesn't exist), check if all the traits have is_valid return true and debugmsg if there are ones that don't
  • Remove unneeded, item-specific bits from the new flag_trait
  • Create a json with the new flag traits - they will need to be declared now, meaning that someone can look them up and you can comment on what they do

After this is done, trait flags will use quick int comparisons instead of much slower string ones, so you could have a ton of traits in a ton of mutations.

@chaosvolt
Copy link
Member Author

Ah, thanks. I'll try to work on it when I get home later today, as currently getting ready to head out.

@chaosvolt chaosvolt marked this pull request as draft June 2, 2023 21:30
@chaosvolt
Copy link
Member Author

Okay, I've committed what I've got so far, with the flag_trait files and injections into init.cpp, will tinker with this more then see when I can test it.

@scarf005 scarf005 requested a review from Coolthulhu June 2, 2023 21:32
src/flag_trait.cpp Outdated Show resolved Hide resolved
@Coolthulhu
Copy link
Member

Apart from what I mentioned above, it looks good.

@chaosvolt
Copy link
Member Author

Ah thanks, will work on that then resume testing in a bit then, got woke up and was about to test something else in a moment.

@chaosvolt
Copy link
Member Author

Let's see, still some errors currently that don't seem to make sense. Also it seems that regular flags don't have a null defined in string_id_null_ids.cpp hence the declaration in the file itself.

@olanti-p
Copy link
Contributor

it seems that regular flags don't have a null defined in string_id_null_ids.cpp

They define the null flag as a constant flag_NULL in the header, and it should also be moved to string_id_null_ids.cpp.
This way, anyone who would want a null flag_str_id will be able to use flag_str_id::NULL_ID() instead of searching for the constant or, failing that, coming up with a new one.

src/flag_trait.h Outdated Show resolved Hide resolved
src/flag_trait.cpp Outdated Show resolved Hide resolved
@chaosvolt
Copy link
Member Author

Ah, I'll try and work on it when I get time, just been juggling a lot IRL lately. @.@

@chaosvolt
Copy link
Member Author

So now I've got the actual definition of trait flags working, I'm poking through and need to find which part is responsible for trait flags checking regular flags, in order to change it...

@scarf005 scarf005 self-assigned this Sep 23, 2023
@scarf005 scarf005 mentioned this pull request Sep 23, 2023
chaosvolt added a commit to chaosvolt/cdda-arcana-mod that referenced this pull request Sep 23, 2023
@chaosvolt
Copy link
Member Author

Alright, I've got the code actually checking for if a trait flag exists and confirmed it actually works.

That should leave the "check that conflicting trait flags actually works" as the last step. I'm still not 100% sure how to format that.

@scarf005 scarf005 requested a review from olanti-p September 25, 2023 12:31
Copy link
Member

@scarf005 scarf005 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CV, could you compile test this again, it worked on my machine but i'd like to be extra sure

@chaosvolt
Copy link
Member Author

chaosvolt commented Sep 26, 2023

CV, could you compile test this again, it worked on my machine but i'd like to be extra sure

Compiled and load-tested with no issues, did the delicious people meat test too, as before it warns if you have no traits, doesn't warn if you have psychopath.

Should I repeat more of the tests in the OP?

@scarf005
Copy link
Member

no, i think it's good.

@scarf005 scarf005 added this pull request to the merge queue Sep 26, 2023
Merged via the queue into cataclysmbnteam:upload with commit dc43e23 Sep 26, 2023
@chaosvolt chaosvolt deleted the trait-flag-improvement branch September 26, 2023 04:54
@scarf005
Copy link
Member

image

...i think this PR broke every existing appearance mods.

@chaosvolt
Copy link
Member Author

Yeah, third-party mods that use mutation flags for anything need an update to define the flags they use in JSON. I had updates set aside for my mods affected by this and notified Kenan, but others will need to be made aware of it. @.@

scarf005 added a commit to scarf005/Cataclysm-BN that referenced this pull request Sep 28, 2023
* Streamline has_trait_flag behavior

* Start work on it

* Update type_id.h

* Update the thing

* Commit latest fixes

* Update flag_trait.h

* Commit what I have for now

* style(autofix.ci): automated formatting

* Update condition.cpp

* And that should be the basics of the code

* style(autofix.ci): automated formatting

* Start up the JSON work next

* Update flags_mutation.json

* Update JSON_FLAGS.md

* style(autofix.ci): automated formatting

* Update flags_mutation.json

* Update string_id_null_ids.cpp

* Apply suggestions from code review

Co-authored-by: Olanti <[email protected]>

* Do some of the suggested things

* Work on part of the requests

* Add the validation thingy

* refactor: extract `trait_flag_str_id` for `PRED`

* refactor: remove `conflicts` usage

Co-authored-by: olanti-p <[email protected]>

* refactor: extract other trait flag ids

Co-authored-by: olanti-p <[email protected]>

* style(autofix.ci): automated formatting

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: Olanti <[email protected]>
Co-authored-by: scarf <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants